fwserver: add list resources to GetProviderSchema
#1152
Conversation
bbasata
changed the title
GetProviderSchema
bbasata
force-pushed
the
add-list-to-fwserver-part-ii
branch
from
da1aba6 to
83f0604
Compare
bbasata
force-pushed
the
add-list-to-fwserver
branch
from
c57458a to
586bdca
Compare
bbasata
force-pushed
the
add-list-to-fwserver-part-ii
branch
from
1da956e to
70aac24
Compare
bbasata
force-pushed
the
add-list-to-fwserver
branch
from
586bdca to
f971285
Compare
bbasata
force-pushed
the
add-list-to-fwserver-part-ii
branch
from
70aac24 to
d6776b3
Compare
bbasata
force-pushed
the
add-list-to-fwserver
branch
from
485abb7 to
4dd9564
Compare
bbasata
force-pushed
the
add-list-to-fwserver
branch
from
4dd9564 to
7a0dfb3
Compare
bbasata
force-pushed
the
add-list-to-fwserver-part-ii
branch
4 times, most recently
from
b30df31 to
be455ea
Compare
bbasata
force-pushed
the
add-list-to-fwserver-part-ii
branch
from
be455ea to
7691bb8
Compare
austinvalle
left a comment
austinvalle
left a comment
There was a problem hiding this comment.
LGTM as a first go, left some considerations for future PRs
| continue | ||
| } | ||
|
|
||
| if _, ok := s.resourceFuncs[typeName]; !ok { |
There was a problem hiding this comment.
Is s.resourceFuncs guaranteed to be populated at this point?
There was a problem hiding this comment.
In a roundabout way, yes. The fwserver implementations of GetMetadata and GetProviderSchema both call Server.ResourceFuncs to prime that cache before Server.ListResourceFuncs.
There's room for improvement.
There was a problem hiding this comment.
Maybe we change this to s.ResourceFuncs(ctx)[typeName] ...
| // also validated. | ||
| func (s *Server) ListResourceSchemas(ctx context.Context) (map[string]fwschema.Schema, diag.Diagnostics) { | ||
| listResourceSchemas := make(map[string]fwschema.Schema) | ||
| listResourceFuncs, diags := s.ListResourceFuncs(ctx) |
There was a problem hiding this comment.
This will give us the list resource implementations actually defined in ListResources(), but couldn't there also be implementations in Resources() as well? Potentially conflicting 👀
same thought for metadata
There was a problem hiding this comment.
My current thinking on this is: Framework only needs to look at ListResources().
If a provider type implements both resource.Resource and list.ListResource, then they add a NewMyResource function in two places: Resources() and ListResources().
There's room for improvement...
| Path: path.Root(attributeName), | ||
| } | ||
|
|
||
| diags.Append(fwschema.IsReservedResourceAttributeName(req.Name, req.Path)...) |
There was a problem hiding this comment.
Just a reminder (since it's also relevant for state storage), we should double check if there are any additional fields that should be protect from shadowing that aren't the same as managed resources.
For example, state_store blocks will only need to ensure there is no provider attribute in the schema.
2 participants
Description
To merge after #1151. This pull request adds list resource metadata to the protocol-independent framework server implementation of
GetProviderSchema.For a focused start, I have defined
list.Schema.StringAttributeand no other attribute types or block types. Then, the remaining schema definition can follow in another focused pull request.Rollback Plan
Changes to Security Controls
None